-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: gelato and local signers #1346
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No major comments from me. I see there are still some TODOs, but not sure if you want to get them addressed in this PR or a follow-up.
Thanks for reviewing! Yea the TODOs will be handled in follow up PRs. I opened separate tickets for them as well |
const baseUrl = resolveVercelEndpoint(true); | ||
const response = await queue.enqueueJSON({ | ||
retries: 3, | ||
contentBasedDeduplication: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this solve nonce collision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea that's the idea. by using a queue and setting parallelism to the number of signers we have, we should be able to mitigate nonce collisions on a single chain.
strategy: RelayStrategy; | ||
}) { | ||
const strategyName = strategy.strategyName; | ||
const queue = getRelayRequestQueue(strategyName, request.chainId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is each queue/strategy using different EOAs? If not, how do we avoid nonce collisions between txns submitted within the same block between these queues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it depends on the strategy. for example, if using Gelato, we don't need to handle nonce collisions because they take care of it. if using local signers, then we need to implement a mechanism/handler that's able to prevent nonce collisions.
break; | ||
} | ||
|
||
await new Promise((resolve) => setTimeout(resolve, 1_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do you think there's value in setting the polling interval to the block time for the chain? or maybe make this an ENV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yea block time could make sense actually. a bit hesitant on using an env var though
data: encodedCalldata, | ||
from: wallet.address, | ||
}; | ||
const tx = await wallet.sendTransaction(txRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to simulate this tx first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sendTransaction
actually does an internal estimateGas
call already if no gasLimit
is provided
|
||
export function encodeCalldataForRelayRequest(request: RelayRequest) { | ||
let encodedCalldata: string; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a todo for transfer with auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep will do
try { | ||
assert(request.query, RelayRequestStatusSchema); | ||
|
||
const cachedRelayRequest = await getCachedRelayRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we handle if a request hash is not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes good call!
api/swap/permit/_utils.ts
Outdated
deposit: depositTypedData.eip712, | ||
permit: { | ||
...permitTypedData.eip712, | ||
message: stringifyBigNumProps(permitTypedData.eip712.message), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since stringifyBigNumProps
runs recursively, can we call it once at the root of this object, just in case we might miss a value, or if we add fields in later PRs we might forget to also serialize that object correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep good point. I hoisted it up to api/swap/permit now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I only have a few questions, for my own understanding
Closes ACX-3568, ACX-3569 and ACX-3570
This PR builds on top of #1345 and implements two relay strategies:
The high-level lifecycle of a relay request is implemented as follows:
POST /relay
requestHash
POST /relay/jobs/process
GET /relay/status?requestHash
POST /relay
as well if the user wants a long-running atomic request